PRDs/20251224-codegen-quality/implementation-plan.md (1)

43-46: Minor: Inconsistent path format in documentation.

The document uses full paths in lines 43–46 (e.g., databind/src/main/java/dev/metaschema/databind/codegen/typeinfo/INamedInstanceTypeInfo.java) but placeholder paths in lines 172–179 (e.g., databind/src/main/java/.../config/binding/MetaschemaBindings.java). For clarity and consistency, standardize on one format throughout the document.

🔎 Suggestion: Use consistent path format

For readability, prefer either:

• Option A (full paths): databind/src/main/java/dev/metaschema/databind/codegen/config/binding/MetaschemaBindings.java
• Option B (abbreviated): databind/src/main/java/.../codegen/config/binding/MetaschemaBindings.java

Apply chosen format uniformly across all file path references.

Also applies to: 115-119, 172-179

PRDs/20251208-module-builder/implementation-plan.md (1)

1-10: Clarify prerequisite dependency on PR #618.

The implementation plan uses the new dev/metaschema package paths throughout, which assumes PR #618 (package rename) is merged. Consider adding a brief header note or dependency statement (e.g., "Prerequisite: PR #618 package rename must be merged") to make this clear to future readers and link this plan to the broader context.

core/src/main/java/dev/metaschema/core/metapath/cst/NamedFunctionReference.java (1)

19-23: Pre-existing documentation inconsistency: "variable reference" should be "named function reference".

The class Javadoc says "variable reference" but links to #id-named-function-ref and this class is NamedFunctionReference. The same inconsistency exists in the getName() method description (line 49). This is a pre-existing issue not introduced by this PR, but worth fixing in a follow-up.

🔎 Suggested documentation fix

 /**
  * The CST node for a Metapath
- * <a href="https://www.w3.org/TR/xpath-31/#id-named-function-ref">variable
- * reference</a>.
+ * <a href="https://www.w3.org/TR/xpath-31/#id-named-function-ref">named
+ * function reference</a>.
  */

And for the getName() method:

   /**
-   * Get the variable name.
+   * Get the function name.
    *
    * @return the name of the referenced function
    */

PRDs/20251228-validation-errors/implementation-plan.md (4)

15-35: Clarify the Format type source and definition.

Line 24 references a Format field in ValidationContext, but the type and its origin are not specified in this plan. Before implementation, confirm where Format is defined (existing enum, new type, etc.) and whether it's already available in the codebase or needs to be created as part of this work.

If Format does not exist, would you like me to help draft its definition?

---

37-57: Ensure PathTracker Javadoc explicitly documents non-thread-safety.

The acceptance criteria correctly acknowledge thread-safety considerations (line 56). When implementing, add explicit Javadoc to PathTracker stating it is not thread-safe and requires external synchronization if shared across threads, or clarify that each thread should maintain its own instance during parsing.

---

239-250: Specify the synthetic URI format for null input fallback.

Task 3.6 mentions using a "synthetic URI" when null is passed, but does not define its format. Before implementation, clarify what this URI should be (e.g., unknown://unknown, a fixed constant, a UUID-based URI). This is important for consistency in error messages tested in Task 2.3 (testErrorWithoutSourceUri).

Would you like me to propose a convention (e.g., URI.create("unknown://unknown")) or is there an existing pattern in the codebase for this?

---

180-196: Clarify the property type grouping strategy in error messages.

Line 187 mentions "Group missing properties by type" and line 195 specifies distinguishing attributes vs. elements for XML. Before implementation, detail the exact grouping criteria: are you grouping by structural role (flags, fields, assemblies) or by serialization type (attributes, elements, properties)? Provide a brief example of the expected error message format for multiple missing properties of different types.

core/src/main/java/dev/metaschema/core/datatype/object/AbstractAmbiguousTemporal.java (1)

63-66: Fix typo in toString() output (pre-existing).

Line 65 contains a typo: "(abiguous)" should be "(ambiguous)". This affects the string representation used in debugging and logging.

🔎 Proposed fix

 @Override
 public String toString() {
-  return getValue().toString() + (hasTimeZone() ? "" : "(abiguous)");
+  return getValue().toString() + (hasTimeZone() ? "" : "(ambiguous)");
 }

PRDs/20251215-shell-completion/implementation-plan.md (1)

490-490: Clarify enum completion type registration timing and location.

Phase 5 proposes adding .type(Format.class) to option definitions, and the acceptance criteria (lines 559-560, 573) mention registering enum types with CompletionTypeRegistry.registerEnum(). However, the plan doesn't specify:

• When these registrations occur (application startup, static initializer, lazy on first use?)
• Where they're called (main method, command initialization, registry static block?)
• Who registers them (framework vs. user code for custom enums?)

For example, is there a designated place in MetaschemaCommands or a startup hook where CompletionTypeRegistry.registerEnum(Format.class) would be called before shell completion is invoked?

Would you like me to propose a registration mechanism (e.g., static initializer in a registry loader class, or a command initialization hook)?

Also applies to: 559-560, 573-573

core/src/main/java/dev/metaschema/core/mdm/IDMAssemblyNodeItem.java (1)

28-28: Package reference updated correctly; pre-existing documentation issue noted.

The FQN update to dev.metaschema.core.model.IFlagInstance is correct. However, this line references a newFlag method that does not exist in this interface—only newField and newAssembly are defined. This appears to be a pre-existing documentation error.

🔎 Suggested documentation fix

- * {@link #newFlag(dev.metaschema.core.model.IFlagInstance, IAnyAtomicItem)},
+ * {@link IDMModelNodeItem#newFlag(dev.metaschema.core.model.IFlagInstance, IAnyAtomicItem)},

Or if newFlag is inherited from a parent interface, verify the correct reference. If the method doesn't exist anywhere, consider removing this line.

PRDs/20251231-saxon-jdom2-removal/implementation-plan.md (2)

87-101: Acceptance criteria for IndentingXMLStreamWriter are comprehensive but lack edge-case coverage.

The criteria correctly emphasize text preservation and mixed content tests (lines 63–68, 70–75), especially for XHTML in xs:documentation. However, consider adding test cases for:

• Empty CDATA sections and nested CDATA (not just "not indented internally")
• Processor instructions with attributes
• Namespace prefix declarations on nested elements
• Very deeply nested structures (nesting stress test)

These additions would improve confidence that the indenter handles real-world XSD schema documentation safely.

---

123-150: Characterization test approach for Phase 2 is sound but raises integration risk.

The plan (lines 125–149) correctly uses characterization tests to verify behavioral equivalence before deleting JDOM2 classes. However, line 143 suggests parameterizing tests to cover both old and new implementations:

Update tests to use new implementations (or parameterize to test both)

This is vague. If tests are parameterized, the old JDOM2 implementation classes must remain available during Phase 2 verification. Clarify:

• Will JDOM2 classes be deleted only after all parameterized tests pass?
• If test classes are in separate files (XmlSchemaLoaderTest.java, DomDatatypeContentTest.java), how is equivalence verified without side-by-side comparison?

A clearer approach: Use a parameterized test that accepts a supplier/factory for the implementation, allowing Phase 2 to prove equivalence before deletion.

PRDs/20251217-parallel-constraint-validation/implementation-plan.md (2)

140-247: ParallelValidationConfig API design is sound, but clarify the threshold rationale.

The class correctly distinguishes between internal (owned) and external (unowned) executors in close(), uses appropriate null-checking, and provides sensible factory methods. The SEQUENTIAL constant as a singleton is a good pattern for avoiding unnecessary object allocation.

However, consider adding a brief Javadoc note explaining why PARALLEL_THRESHOLD = 4 was chosen—whether based on empirical testing, heuristics, or a conservative estimate. If this value should be tunable in the future, consider noting that as a potential enhancement.

---

642-669: Sorting findings on every call may have performance implications.

The getFindings() method (lines 642–646) sorts the queue every time it is called. For documents with many constraint violations, repeated calls to this method will repeatedly sort the findings list. Consider documenting this behavior or, if performance becomes a concern, caching the sorted list with invalidation on new findings.

If sorting performance is not a bottleneck in practice, document the design decision in the Javadoc so future maintainers understand why sorting occurs on every call rather than on-demand or incrementally.

PRDs/20251231-saxon-jdom2-removal/implementation-plan.md (2)

123-165: Characterization testing approach is sound; consider parameterized test guidance.

The two-step approach (characterization tests against JDOM2, then verification against new DOM implementation) follows best practices for behavioral equivalence verification. Line 143 mentions "parameterize to test both," which is wise for confidence; clarifying in the acceptance criteria that the goal is behavioral equivalence (not dual-implementation testing) would help implementers understand the expected test lifecycle.

---

396-412: XPath namespace handling example is clear; may need multi-namespace guidance during implementation.

The example covers the xs namespace (XSD) correctly. If XSD schemas in the codebase use other namespaces (XHTML in documentation, etc.), ensure the NamespaceContext implementation handles multiple prefixes. This can be deferred to implementation but may be worth calling out in phase 2 acceptance criteria if known namespaces are identified.

core/src/main/java/dev/metaschema/core/metapath/function/CalledContext.java (1)

102-107: Consider removing redundant function equality check.

The equals method checks function equality twice: Line 102 verifies !getFunction().equals(other.getFunction()) and returns false if not equal, but then Line 105 checks Objects.equals(function, other.function) again as part of the final return statement. Since the first check already returned false when functions don't match, the second check is redundant.

🔎 Proposed fix to remove redundancy

   CalledContext other = (CalledContext) obj;
-  if (!getFunction().equals(other.getFunction())) {
-    return false;
-  }
-  return Objects.equals(function, other.function)
+  return Objects.equals(function, other.function)
       && Objects.equals(arguments, other.arguments)
       && Objects.equals(contextItem, other.contextItem);

core/src/main/java/dev/metaschema/core/datatype/markup/flexmark/impl/FixedEmphasisDelimiterProcessor.java (2)

26-26: Track upstream fix for potential cleanup.

The TODO indicates this class is a temporary workaround pending the merge of flexmark-java#580.

Would you like me to open an issue to track monitoring the upstream PR and removing this workaround once it's merged?

---

41-67: Consider adding Javadoc for implementation-specific behavior.

The override methods getDelimiterUse and process contain non-trivial implementation-specific logic (e.g., the "multiple of 3" rule, emphasis wrapping decisions) that differs from standard delimiter processing. While inherited Javadoc covers the interface contract, documenting these implementation details would improve maintainability, especially given this is a temporary workaround for upstream behavior.

Example Javadoc enhancement for getDelimiterUse

+  /**
+   * {@inheritDoc}
+   * <p>
+   * This implementation applies the "multiple of 3" rule for internal delimiter runs
+   * and adjusts delimiter usage based on opener/closer lengths to fix flexmark-java
+   * issue #580.
+   */
   @Override
   public int getDelimiterUse(DelimiterRun opener, DelimiterRun closer) {

CONTRIBUTING.md (1)

110-110: Update specification link to use canonical documentation URL.

The package path update to dev/metaschema/core/metapath/function/library/DefaultFunctionLibrary.java is correct and aligns with the PR's renaming objectives. However, the specification link still uses https://pages.nist.gov/metaschema/ when the project's preferred and canonical documentation URL is https://framework.metaschema.dev/.

Per prior learnings, pages.nist.gov URLs should not be used in documentation; consider updating the specification link to use the canonical framework.metaschema.dev URL.

🔎 Proposed fix to align with canonical documentation URL

-The Metaschema [specification](https://pages.nist.gov/metaschema/specification/syntax/metapath/) and [this conformant library](https://github.com/metaschema-framework/metaschema-java) library implement the Metapath functions required by the specification. See the library's [registry class with an inventory listing of the functions](https://github.com/metaschema-framework/metaschema-java/blob/main/core/src/main/java/dev/metaschema/core/metapath/function/library/DefaultFunctionLibrary.java) supported in the most current release.
+The Metaschema [specification](https://framework.metaschema.dev/specification/syntax/metapath/) and [this conformant library](https://github.com/metaschema-framework/metaschema-java) library implement the Metapath functions required by the specification. See the library's [registry class with an inventory listing of the functions](https://github.com/metaschema-framework/metaschema-java/blob/main/core/src/main/java/dev/metaschema/core/metapath/function/library/DefaultFunctionLibrary.java) supported in the most current release.

Based on learnings from PR #605, framework.metaschema.dev is the canonical documentation URL for this project.

PRDs/20251215-shell-completion/PRD.md (2)

248-250: Demonstrate the new ExtraArgument type feature in ShellCompletionCommand example.

The PRD introduces type-based completion via ExtraArgument.getType() (lines 215–224), but the example at line 248–250 uses the old 2-argument factory method. To showcase the new capability and align with the design goals, the shell argument itself should specify a completion type:

// Shell enum would be defined as shown on lines 243–246
public enum Shell {
    BASH,
    ZSH
}

// Then register it:
static {
    CompletionTypeRegistry.registerEnum(Shell.class);
}

// And use it in EXTRA_ARGUMENTS:
ExtraArgument.newInstance("shell", true, Shell.class)

This demonstrates that the feature applies not just to Options but also to ExtraArguments, making the design more cohesive.

---

184-188: Consider documenting shell built-in function availability.

The PRD references shell built-in completion functions (_filedir, _files, _urls) without noting version or environment requirements. These are typically available in bash-completion package (v2.8+) and zsh completion system, but availability varies by system.

Consider adding a compatibility note or fallback behavior (e.g., graceful degradation for older shells). Alternatively, document minimum shell versions or suggest that end users install bash-completion package if these functions are not available.

PRDs/20251228-validation-errors/implementation-plan.md (1)

254-275: Verification phase covers implementation but may miss broader integration testing.

The verification plan runs databind tests (mvn -pl databind test on line 257) and full CI build. However, since validation contexts flow through JSON and XML readers that are used across the codebase, consider adding integration tests in the core or testing modules to ensure ValidationContext propagation works correctly end-to-end. The full build will catch compilation errors but not all functional integration issues.

core/src/main/java/dev/metaschema/core/datatype/adapter/TimeWithTZAdapter.java (1)

6-6: Update coding guidelines to reflect new package naming convention.

The coding guidelines currently state: "Follow package naming convention gov.nist.secauto.metaschema.* for all Java packages", but this PR establishes dev.metaschema.* as the new canonical convention. The guidelines should be updated to avoid confusion in future reviews.

Based on learnings, the canonical package naming convention is dev.metaschema.* as of PR #618.

Would you like me to help draft updated coding guidelines that reflect the new dev.metaschema.* package naming convention?

core/src/main/java/dev/metaschema/core/datatype/markup/package-info.java (1)

50-50: LGTM! Package declaration correctly updated to new namespace.

The package declaration has been properly updated from gov.nist.secauto.metaschema.core.datatype.markup to dev.metaschema.core.datatype.markup, correctly implementing the package rename objective.

Note: The coding guidelines currently state "Follow package naming convention gov.nist.secauto.metaschema.* for all Java packages", but this guideline is outdated. Based on learnings, the canonical convention is dev.metaschema.* as of this PR (#618). The coding guidelines should be updated to reflect this new convention.

core/src/main/java/dev/metaschema/core/metapath/function/FunctionService.java (1)

35-35: Address the FIXME comment.

The FIXME suggests renaming getInstance() to instance() for consistency.

Would you like me to open a follow-up issue to track this naming improvement, or would you prefer to address it in this PR?

core/src/main/java/dev/metaschema/core/metapath/cst/type/Castable.java (1)

6-6: Update coding guidelines to reflect new package convention.

The coding guidelines currently specify:

Follow package naming convention gov.nist.secauto.metaschema.* for all Java packages

This is now outdated as of PR #618, which establishes dev.metaschema.* as the canonical naming convention. The guidelines document should be updated to reflect this breaking change.

Would you like me to generate a script to locate and update the coding guidelines file?

core/src/main/java/dev/metaschema/core/metapath/function/CalledContext.java (1)

93-108: Optional: Simplify redundant equality check in equals().

The method checks function equality twice:

1. Line 102-104: early return if functions aren't equal
2. Line 105: re-checks Objects.equals(function, other.function)

Since line 105 is only reached when functions are already known to be equal, the check is redundant. Consider simplifying:

🔎 Proposed simplification

   @Override
   public boolean equals(Object obj) {
     if (this == obj) {
       return true;
     }
     if (obj == null || getClass() != obj.getClass()) {
       return false;
     }
     CalledContext other = (CalledContext) obj;
-    if (!getFunction().equals(other.getFunction())) {
-      return false;
-    }
-    return Objects.equals(function, other.function)
-        && Objects.equals(arguments, other.arguments)
-        && Objects.equals(contextItem, other.contextItem);
+    return Objects.equals(function, other.function)
+        && Objects.equals(arguments, other.arguments)
+        && Objects.equals(contextItem, other.contextItem);
   }

PRDs/20251231-saxon-jdom2-removal/implementation-plan.md (4)

1-35: Clear TDD mandate, but phase dependencies need explicit documentation.

The plan correctly enforces TDD as mandatory (line 32), which aligns with project learnings. However, the phase sequencing (line 9: "tightly coupled, cannot function independently") implies hidden prerequisites—e.g., Phase 3 depends on Phase 1 completion, but this is not explicitly stated in Phase 3's requirements. Consider adding a "Prerequisites" subsection to each phase listing prior phases that must be complete.

---

54-101: Phase 1 design is sound; clarify "whitespace-only" text detection in implementation.

The test scenarios for IndentingXMLStreamWriter are comprehensive and correctly identify the critical case: preserving text content without spurious indentation. The stack-based state machine (lines 320–363) elegantly handles nested mixed-content. However, the acceptance criteria should clarify what "whitespace-only" means during implementation. For example:

• Is a text node of "\n " (newline + indent) considered whitespace-only?
• What about "\t"?

This distinction affects whether a parent's hasText flag is set and thus whether child elements receive indentation. Consider adding a test case for this edge case (e.g., "Test that whitespace-only text nodes do not trigger mixed-content mode") to the Phase 1 acceptance criteria.

---

104-165: Characterization-test-first strategy is excellent; clarify DocumentBuilderFactory namespace configuration.

The Phase 2 approach of writing characterization tests against JDOM2, verifying they pass, then replacing with new implementation is a strong TDD strategy for behavioral equivalence. However, a critical detail is missing: DocumentBuilderFactory must be configured with setNamespaceAware(true) to correctly handle XPath namespace queries (line 132). Without this, XPath queries like /xs:schema/xs:simpleType will fail because the default namespace context is not populated.

Recommendation: Add an explicit acceptance criterion:

• "Ensure DocumentBuilderFactory.setNamespaceAware(true) is set in XmlSchemaLoader to support namespace-aware XPath queries."

Additionally, the test scenario at line 129 should specify what assertions are expected (e.g., "verify element count matches JDOM2 result", "verify element names in order", etc.) to ensure characterization tests capture all relevant behavior.

---

243-272: Phases 4–6 are clear; minor clarification on Phase 5 test redundancy.

Phases 4 (configuration), 5 (test updates), and 6 (verification) are well-structured. However, Phase 5 mentions updating XmlSuiteTest to replace StAXEventBuilder with DocumentBuilder (line 256), but Phase 2 already defines characterization tests that test both JDOM2 and new DOM implementations. Clarify whether:

1. Phase 5 updates are separate integration tests, or
2. Phase 5 reuses/refactors Phase 2 characterization tests to cover the broader XmlSuiteTest suite.

This avoids duplicate test-writing effort and keeps the plan efficient.

cli-processor/src/main/java/dev/metaschema/cli/processor/CallingContext.java (1)

125-127: Consider adding Javadoc to package-private methods for consistency.

While the coding guidelines only mandate Javadoc for public/protected members (which are fully covered), some package-private methods have Javadoc (e.g., getOptionsList(), getExtraArgs()) while others don't (getCalledCommands(), toOptions()). Adding brief documentation to these methods would improve consistency and maintainability.

Also applies to: 144-150

PRDs/20251217-parallel-constraint-validation/implementation-plan.md (3)

1041-1043: PARALLEL_THRESHOLD constant placement.

The PARALLEL_THRESHOLD = 4 constant is defined at line 1012 but the decision to parallelize based on sibling count (line 1042: item.modelItems().count() >= PARALLEL_THRESHOLD) is good. However, confirm:

1. This threshold is appropriate for typical document structures
2. The threshold makes sense relative to JVM overhead for task submission

Consider making the threshold configurable via ParallelValidationConfig for future tuning:

private int getParallelThreshold() {
  return parallelConfig.getParallelThreshold();
}

---

1315-1330: Integration tests are well-planned but marked as placeholders.

Task 7 shows integration test scenarios (lines 1315-1330) but the test bodies are incomplete. Recommend:

1. Implementing these as soon as the infrastructure changes are complete
2. Using test fixtures from Task 1-6 to create realistic validation scenarios
3. Adding performance assertions (e.g., parallel faster than sequential on large documents)

Would you like me to help generate complete integration test implementations that validate end-to-end parallel constraint validation behavior?

---

807-807: Reconsider the necessity of alternative to Collections.synchronizedList for keyRefItems.

The code at line 709 uses Collections.synchronizedList(new ArrayList<>()) to store key references during parallel validation. While the concern about synchronizedList's locking behavior is generally valid, the actual usage pattern mitigates this concern:

• Write phase: keyRefItems.add() is called concurrently during parallel validation
• Read phase: Sequential iteration occurs in finalizeValidation() (lines 977-989) only after all parallel validation completes
• No concurrent access: Reads and writes never overlap

Since iteration happens exclusively in a sequential finalization phase, Collections.synchronizedList is acceptable here. However, CopyOnWriteArrayList would be slightly more appropriate for this write-before-read pattern, as it would eliminate lock contention during the sequential finalization phase without the overhead of synchronization on every operation.

The current choice is not suboptimal given the actual access pattern, though a minor optimization is possible.

PRDs/20251231-saxon-jdom2-removal/implementation-plan.md (1)

396-412: Verify XPath namespace handling tests cover schema-relevant namespaces.

The NamespaceContext example is helpful, but Phase 2 acceptance criteria (lines 152–165) should explicitly require tests for namespace-qualified XPath queries commonly used in schema processing (e.g., /xs:schema/xs:simpleType with the XSD namespace registered). This prevents runtime namespace-resolution failures when deployed.

PRDs/20251206-build-cleanup/implementation-plan.md (1)

274-421: Javadoc PRs (7-12) lack concrete deliverables and definition of done.

PRs 7-12 define scope directories and estimated file counts ("~40-50") but lack:

• Exact file lists to document
• Clear Javadoc coverage targets (zero warnings? 100% method docs?)
• Guidance on what qualifies as "complete" for acceptance

Recommend creating a Javadoc audit or tool output listing:

• Current warnings per module
• Target state (e.g., zero warnings)
• Specific files requiring attention

This will make PRs actionable and measurable before implementation.

core/src/main/java/dev/metaschema/core/datatype/markup/MarkupLine.java (1)

6-6: Update coding guidelines to reflect the new package naming convention.

The coding guidelines currently state: "Follow package naming convention gov.nist.secauto.metaschema.* for all Java packages", but this PR establishes dev.metaschema.* as the new canonical convention. Please update the coding guidelines document (docs/javadoc-style-guide.md or wherever the guideline is maintained) to reflect this breaking change.

As per coding guidelines (which need updating), the old convention was gov.nist.secauto.metaschema.*.

Do you want me to help identify which documentation files need to be updated to reflect the new package naming convention?

PRDs/20251208-constraint-builder/implementation-plan.md (1)

1-20: Add explicit inter-PR dependency documentation.

The PR strategy section should document dependencies on the companion module-builder PRD (specifically that both PRDs will add methods to IModuleMockFactory). This will clarify sequencing and merge order for reviewers and maintainers.

PRDs/20251208-module-builder/implementation-plan.md (1)

35-35: Clarify module definition wiring in acceptance criteria.

PR 2 acceptance criteria (lines 35, 39–41) state that toDefinition(IModule) methods set getContainingModule() and that toModule() "builds all definitions with proper module wiring." Specify the exact mechanism: Do builders call a setter on the definition object, or does the module constructor wire the reference? This will help reviewers validate implementation completeness.

Also applies to: 39-41

core/src/main/java-templates/dev/metaschema/core/MetaschemaJavaVersion.java (1)

22-55: Consider adding @nonnull annotations for null-safety guarantees.

The getter methods return non-null String constants, but lack explicit @NonNull annotations. Adding these annotations would strengthen the API contract and enable static analysis tools to catch potential issues in calling code.

🔎 Proposed addition of @nonnull annotations

Import the annotation at the top of the file:

 package dev.metaschema.core;
 
 import dev.metaschema.core.util.IVersionInfo;
+
+import edu.umd.cs.findbugs.annotations.NonNull;

Then annotate the return types:

   @Override
+  @NonNull
   public String getName() {
     return NAME;
   }
 
   @Override
+  @NonNull
   public String getVersion() {
     return VERSION;
   }
 
   @Override
+  @NonNull
   public String getBuildTimestamp() {
     return BUILD_TIMESTAMP;
   }
 
   @Override
+  @NonNull
   public String getGitOriginUrl() {
     return ORIGIN;
   }
 
   @Override
+  @NonNull
   public String getGitCommit() {
     return COMMIT;
   }
 
   @Override
+  @NonNull
   public String getGitBranch() {
     return BRANCH;
   }
 
   @Override
+  @NonNull
   public String getGitClosestTag() {
     return CLOSEST_TAG;
   }

core/src/main/java/dev/metaschema/core/metapath/cst/type/Treat.java (1)

22-25: Pre-existing Javadoc reference may be incorrect.

The class Javadoc references the XPath 3.1 "cast as" operator (#id-cast), but this is the Treat class which implements the "treat as" expression. The correct XPath 3.1 reference would be #id-treat. This appears to be a pre-existing issue not introduced by this PR's namespace changes.

🔎 Proposed fix

 /**
  * A compact syntax tree node that supports the Metapath
- * <a href="https://www.w3.org/TR/xpath-31/#id-cast">"cast as" operator</a>.
+ * <a href="https://www.w3.org/TR/xpath-31/#id-treat">"treat as" expression</a>.
  */

core/src/main/java/dev/metaschema/core/datatype/adapter/DateTimeWithTZAdapter.java (1)

26-26: Consider using the preferred documentation URL.

The Javadoc references https://pages.nist.gov/metaschema/..., but the project's preferred and canonical documentation domain is https://framework.metaschema.dev/. Consider updating to use the canonical URL for consistency with project documentation standards.

Based on learnings, framework.metaschema.dev is the official reference domain for Metaschema framework documentation.

PRDs/20251215-shell-completion/PRD.md (2)

122-138: Document enum constant name handling in CompletionTypeRegistry.forEnum().

Line 137 converts enum constant names to lowercase for completion generation:

.map(String::toLowerCase)

This is a sensible default, but should be documented explicitly in the Javadoc or in the PRD's type mapping guide to clarify that users can expect lowercase enum values in completions (e.g., Format.XML → completion offers xml). This prevents surprises if a tool or script relies on exact case matching.

---

56-78: Add security/escaping guidance to ICompletionType implementers.

Custom ICompletionType implementations (like user-defined completion types) should properly escape or validate their bash/zsh output to prevent injection. Add to the ICompletionType Javadoc:

Security Note: Implementations must ensure returned bash/zsh completion code is properly escaped and safe. Avoid embedding unsanitized strings directly into shell syntax. Use shell-safe values (alphanumeric, -, _) or properly quote complex values.

This prevents a third-party completion type from accidentally or maliciously generating unsafe shell code.

Also applies to: 140-150

PRDs/20251217-parallel-constraint-validation/implementation-plan.md (1)

140-247: ParallelValidationConfig: solid design with proper ownership semantics.

The class correctly distinguishes between:

• Internal executor (created via withThreads(int)): owned by config, shut down by close()
• External executor (provided via withExecutor(ExecutorService)): not shut down, caller retains ownership

Implementation notes:

• withThreads(1) correctly delegates to SEQUENTIAL to avoid unnecessary object creation (line 190-191)
• Lazy initialization of internal executor with double-checked locking (lines 218-224) is appropriate for the single-threaded validation context
• close() properly handles thread interruption and uses awaitTermination(60, TimeUnit.SECONDS) (line 238)
• NullPointerException from Objects.requireNonNull() is standard for null-safety (line 172)

Potential improvement: Consider whether 60-second termination timeout is appropriate for all use cases, or if it should be configurable. This is a minor design consideration, not a defect.

PRDs/20251208-constraint-builder/implementation-plan.md (1)

56-57: Clarify IModuleMockFactory package location with module-builder PRD.

A previous review flagged a critical conflict: this plan places IModuleMockFactory.java at dev/metaschema/core/testsupport/builder/ (line 56), but the sibling module-builder PRD places it at dev/metaschema/core/testing/model/. Both plans claim to modify the same class with different methods (constraintSet() vs. module()).

Before proceeding, confirm:

1. Which location is canonical?
2. Should both methods coexist on the same class or be split?
3. Is this PRD's location decision coordinated with the module-builder PRD?

.github/workflows/build.yml (1)

300-301: Critical: Exit code logic may still be inverted (past review issue appears unresolved).

A past review comment (marked "Addressed in commit cc4aac8") flagged that lychee uses:

• Exit code 2 for broken links (actual link-check failures)
• Exit code 1 for runtime/config errors (including timeouts)

However, the current code still checks exit_code == 1 to trigger issue creation and job failure, with comments stating 1=errors found, 2=timeouts only. If the past review's documentation reference is correct, this logic is inverted:

• Broken links (exit 2) will NOT create issues or fail the job
• Timeouts (exit 1) WILL create issues and fail the job

This defeats the purpose of separating timeout handling from genuine link failures. Please verify the current lychee exit code semantics and correct the branching logic if needed.

What are the exit codes used by lychee link checker version 0.14 or later?

Based on learnings: The prior review at this location cited lychee documentation confirming exit code semantics, and this issue was marked resolved. If the current code is correct, the lychee behavior may have changed or the prior review was mistaken—verification is needed.

Also applies to: 310-312

Also applies to: 310-312

core/src/main/java/dev/metaschema/core/mdm/impl/AbstractDMNodeItem.java (1)

15-15: Optional: Fix typo in Javadoc.

The word "assmebly" should be "assembly". This is a pre-existing typo, not introduced by this PR, but could be corrected for clarity.

🔎 Proposed fix

- * This abstract Metapath assmebly node item implementation supports creating a
+ * This abstract Metapath assembly node item implementation supports creating a

PRDs/20251224-codegen-quality/implementation-plan.md (2)

115-119: Clarify path documentation for consistency.

The "Files to Modify" section (lines 115–119) explicitly lists paths using the dev/metaschema/ package convention, but the "Files Actually Modified" section (lines 167–180) uses ellipsis abbreviations (e.g., databind/src/main/java/.../config/binding/) that obscure the actual package namespace.

For readers unfamiliar with the repository structure and to ensure consistency with the canonical dev.metaschema.* convention, consider either:

1. Expanding all abbreviated paths to match the explicitness of lines 115–119, or
2. Adding a clear pattern explanation at the document start (e.g., "Ellipses represent dev/metaschema/databind/codegen/...").

Also applies to: 167-180

---

1-10: Add context statement about package naming pre-condition.

This implementation plan documents work that assumes Java packages have been (or are being) renamed from gov.nist.secauto.metaschema.* to dev.metaschema.* (PR #618). Adding a brief statement at the beginning (e.g., in the Prerequisites section) that explicitly names this pre-condition would help readers unfamiliar with PR #618 understand the full context.

Example addition to Prerequisites (after line 11):

• Package rename to dev.metaschema.* (PR #618) must be merged before this plan is executed

This is optional and primarily a documentation clarity improvement.

PRDs/20251231-saxon-jdom2-removal/implementation-plan.md (2)

125-137: Add verification step for characterization tests against XSD specification.

The characterization test approach (test JDOM2 behavior, then verify new implementation matches) is sound, but risks perpetuating JDOM2 bugs if they exist.

Recommend adding an explicit step to validate both implementations against the W3C XSD specification for correctness (namespace handling, XPath 1.0 semantics for /xs:schema/xs:simpleType, etc.), rather than relying solely on behavioral equivalence with the original.

---

414-422: DOM element serialization approach is valid; consider adding error handling discussion.

The Transformer approach for serializing DOM elements to XMLStreamWriter is correct, but the plan should note potential error conditions:

• TransformerFactory.newInstance() can raise TransformerFactoryConfigurationError
• transform() can raise TransformerException if the transformation fails
• StAXResult compatibility depends on the Transformer implementation

Recommend adding a note in Phase 2 or 5 about error handling strategy (retry logic, logging, fallback behavior).

PRDs/20251215-shell-completion/implementation-plan.md (1)

485-533: Clarify type assignments and enum registration in Phase 5.

Lines 516-533 list files to update with .type() calls and provide type assignments (table at lines 523-532). However, the plan does not specify where enum completion types should be registered.

For example, TO_OPTION should use Format.class type, but the plan should clarify:

1. Where Format.class completion type is registered (e.g., in command initialization, static block, or SPI)
2. Whether other custom enum types need similar registration
3. If registration happens in MetaschemaCommands or elsewhere

Add explicit registration locations to Phase 5 to ensure completion behavior is wired up for all custom types.

PRDs/20251208-module-builder/implementation-plan.md (1)

10-10: Unresolved: IModuleMockFactory path conflict with constraint-builder PRD requires coordination.

This plan places IModuleMockFactory at dev/metaschema/core/testing/model/IModuleMockFactory.java (lines 10, 56, 75), but the companion constraint-builder PRD specifies the path as dev/metaschema/core/testsupport/builder/IModuleMockFactory.java. Both PRDs must add methods to this class:

• constraint-builder adds constraintSet()
• module-builder adds module() (line 10, 16)

Action required:

1. Confirm the canonical location for IModuleMockFactory (suggest dev/metaschema/core/testsupport/builder/ to align with naming intent)
2. Update this plan to reference the agreed-upon path
3. Ensure both methods coexist on the same interface
4. Coordinate implementation sequence across both PRs to avoid merge conflicts

Also applies to: 56-56, 75-75

.github/workflows/build.yml (1)

276-276: Fix the TIMEOUT grep pattern - issue from previous review still present.

The pattern ^\* \[TIMEOUT\] is incorrect and was flagged in the previous review. Lychee outputs [TIMEOUT] without an asterisk prefix (consistent with the [ERR] pattern on line 275). The current pattern will never match, causing the timeout warning at lines 283-284 to never display.

🔎 Proposed fix

-          TIMEOUTS=$(grep -c "^\* \[TIMEOUT\]" html-link-report.md 2>/dev/null || echo "0")
+          TIMEOUTS=$(grep -c "^\[TIMEOUT\]" html-link-report.md 2>/dev/null || echo "0")

PRDs/20251217-context-functions/PRD.md (1)

122-124: Clarify size computation strategy: eager vs. lazy evaluation.

Line 124 (NFR-2) states size computation should be "lazy/efficient (avoid computing sequence size unless last() is called)", but the Phase 3 design (line 198) computes size eagerly: int size = retval.size().

Consider documenting whether upfront size computation is acceptable or if deferred evaluation is required. This affects both performance and the FocusContext API design (e.g., whether size is computed on-demand or stored).

Also applies to: 196-210

core/src/main/java/dev/metaschema/core/datatype/markup/flexmark/impl/IMarkupVisitor.java (1)

6-6: LGTM! Package rename aligns with PR objectives.

The package declaration correctly uses the new dev.metaschema.* convention. Note that the coding guidelines still reference the old gov.nist.secauto.metaschema.* convention and should be updated.

Based on learnings, the canonical convention is now dev.metaschema.* as of this PR.

PRDs/20251221-xmlbeans-removal/implementation-plan.md (1)

16-17: Clarify the reference material path for external users.

The reference path databind-modules/target/generated-resources/schema/json/metaschema-model_schema.json points to a Maven build artifact, which requires running a full build to generate. Consider documenting the build step required to generate this file, or providing a stable URL or alternative reference for IDE validation.

PRDs/20251217-context-functions/PRD.md (1)

126-190: Solution design is sound and backward-compatible.

The phased approach (FocusContext → DynamicContext → PredicateExpression → StaticContext) is logical and maintains API compatibility. The distinction between subContext() (preserves focus) and subContext(FocusContext) (sets new focus) is important for correctness, but the table at lines 186–189 combined with the code example at lines 200–210 adequately conveys the intent.

Minor suggestion: When implementing, ensure the Javadoc for subContext() and subContext(FocusContext) clearly distinguishes their behaviors to avoid confusion during integration with FLWOR expressions and other sub-context scenarios.

PRDs/20251215-shell-completion/implementation-plan.md (1)

486-533: Task 5.1: Enum Registration Wiring Left Implicit.

The option type assignments table (lines 522–531) specifies that TO_OPTION, AS_FORMAT_OPTION, and AS_SCHEMA_FORMAT_OPTION should use custom enum types (e.g., Format.class). However, the task does not explicitly document where and when these enum types are registered with CompletionTypeRegistry.registerEnum().

Add a paragraph clarifying the registration strategy: for example, whether enum registration happens at module initialization, in a static block within command classes, or via a centralized configuration method. This ensures implementations don't miss the registration step and enum completions work end-to-end.

PRDs/20251228-validation-errors/implementation-plan.md (4)

44-50: PathTracker implementation is straightforward; clarify thread-safety requirement.

The acceptance criteria (line 56) mention "thread-safe or documented as not thread-safe," but the code example uses ArrayDeque<String> without synchronization. Since PathTracker is used during single-threaded parsing (lines 204, 219), document explicitly that it is not thread-safe and intended for single-threaded parsing contexts only.

Add a Javadoc note to PathTracker explicitly stating: This class is not thread-safe and should only be used within a single parsing thread.

---

185-195: AbstractProblemHandler updates require careful implementation of error message formatting.

Task 3.2 specifies using format-appropriate terminology (attribute/element for XML, property for JSON) and grouping by type. The acceptance criteria are clear, but implementation complexity is higher than suggested. Ensure:

• Error messages are human-readable and include all context (location, path, type)
• Null field values (Issue #205, Task 2.4) don't cause NPE or incomplete messages
• Default values are correctly distinguished from missing required values

Consider extracting error message formatting logic into a separate formatter class (e.g., ValidationErrorFormatter) to keep AbstractProblemHandler focused on exception handling rather than string formatting.

---

242-250: Null URI handling in deserializers is important for robustness.

Task 3.6 specifies using a "synthetic URI" for null cases. Define what a synthetic URI looks like (e.g., file:///unknown, memory:///synthetic?) and ensure:

• Error messages remain informative even with synthetic URI
• Synthetic URIs don't pollute production logs or error reports
• The choice of synthetic URI is documented in code comments

Use a descriptive synthetic URI like synthetic:///deserialization or unknown://source and document the choice in DefaultXmlDeserializer and DefaultJsonDeserializer Javadoc for clarity.

---

302-309: Risk assessment is thorough and mitigations are practical.

The four identified risks (backward compatibility, performance, edge cases, IResourceLocation integration) are relevant. Mitigations are sound. One additional risk to consider: Test data management — ensure that Metaschema test modules (referenced in Phase 2.1) are version-controlled and not generated dynamically, to keep tests reproducible and maintainable.

Add a note to the Risk Assessment clarifying that test Metaschema modules and fixtures are committed to source control and documented for maintainability.

PRDs/20251224-codegen-quality/implementation-plan.md (2)

1-10: Document context: Consider adding a note clarifying relationship to PR #618.

This implementation plan documents phases 1–5 of code generator quality improvements across multiple historical PRs. Since this file is being updated as part of PR #618 (package rename), readers may benefit from a brief introductory note explaining that all file paths and package names herein reflect the new dev.metaschema.* namespace.

For example, add a note after line 3:

> **Note:** This document was updated in PR #618 to reflect the package rename 
> from `gov.nist.secauto.metaschema.*` to `dev.metaschema.*`. All file paths 
> and package names herein use the current canonical namespace.

---

1-1: Minor framing clarification: Document reads as retrospective, not forward-looking plan.

The document is titled "Implementation Plan" but comprehensively documents completed work across five PRs with all acceptance criteria marked [x] and PR status marked ✅ Completed. Readers may be clearer if the title or opening reflects this is a historical reference/retrospective document. For example:

• Current: "Implementation Plan: Code Generator Quality Improvements"
• Suggested: "Implementation Summary: Code Generator Quality Improvements"
  or
• Add a subtitle: "Completed phases 1–5 across PRs #577, #584, #593"

This helps readers immediately understand the document is a record of completed work, not a forward-facing roadmap.

Also applies to: 26-26, 98-98, 184-184, 252-252, 312-312, 391-402

PRDs/20251217-parallel-constraint-validation/implementation-plan.md (9)

1041-1043: Verify PARALLEL_THRESHOLD constant tuning and documentation.

The threshold of 4 sibling nodes (line 1041–1043) determines when to switch from sequential to parallel traversal. This is a tuning parameter that should be validated:

1. Is 4 optimal for typical document structures and JVM thread pools?
2. Should this be configurable or left as a constant?
3. Add an inline comment explaining why 4 was chosen (CPU cores, typical list sizes, etc.).

Consider documenting this threshold in ParallelValidationConfig Javadoc or in a separate tuning guide for operators.

---

1206-1231: Validate thread count argument parsing and error messages.

The getParallelConfig() helper (lines 1206–1231) parses --threads and validates the count. Ensure:

1. Error messages are actionable (line 1222: "Invalid thread count: " + value should include the reason).
2. The validation at line 1224 rejects 0 and negative values with a clear message (currently done, good).
3. Non-numeric input is caught and reported clearly (line 1221 catches NumberFormatException).

Consider unifying error message formatting with the rest of the CLI for consistency. Also verify that invalid arguments throw InvalidArgumentException as expected by the CLI framework.

---

476-509: FindingCollectingHandler concurrency test helper methods are incomplete.

The test testConcurrentAddFindings() (lines 476–509) calls handler.handleExpectViolation(...) with mock objects created by unimplemented helper methods (lines 493–495: createMockConstraint(), createMockNode(), createMockContext()). Before finalizing this task:

1. Implement the mock helper methods with realistic objects.
2. Verify that the mocks correctly implement the required interfaces.
3. Ensure the test actually exercises the thread-safe code paths in the real handler.

Without these helpers, the test cannot run. Consider using Mockito or similar framework if the real objects are too heavyweight.

I can help generate the mock helper implementations if you provide details on the constraint and node item interfaces. Would you like me to create those?

---

588-594: Incomplete helper methods in FindingCollectingHandler test block further test execution.

Lines 588–594 define stub helper methods addFinding() and addFindingWithPath() with "Implementation using mocks" comments. These methods must be fully implemented before the tests can run. Provide:

1. Real or mocked implementations of the helper methods.
2. Proper mock setup for ConstraintValidationFinding and related interfaces.
3. Test fixtures for consistent mock data across test methods.

Suggest creating a separate TestFixtures or ConstraintTestHelper utility class if mocking becomes complex.

I can generate the full test fixture and helper implementations if you clarify the structure of ConstraintValidationFinding, INodeItem, and related interfaces.

---

720-751: DefaultConstraintValidator thread-safety test is a skeleton without executable code.

The test testConcurrentValueMapAccess() (lines 720–751) creates a validator but does not invoke any actual validation methods. The comment at lines 734–737 says "Call methods that access valueMap" but provides no implementation. This test will pass vacuously and provide no value.

Complete this test with:

1. Real calls to constraint validation methods that exercise valueMap mutations.
2. Assertions that verify no ConcurrentModificationException is thrown.
3. Proper setup of mock node items and constraints to trigger validation.

Same issue exists for testConcurrentIndexKeyRefAccess() (lines 754–784).

I can generate complete implementations of these tests with proper mock setup and actual validator method calls. Would you like me to do that?

---

883-915: ParallelValidationTest is incomplete with commented-out core test logic.

The key test testSequentialAndParallelProduceSameResults() (lines 883–915) has the critical assertions commented out (lines 902–913). Without these assertions, the test does nothing and will pass trivially.

Uncomment and fully implement:

1. The test document creation (line 902).
2. The dynamic context initialization (line 904).
3. The actual validation calls (lines 906–907).
4. The findings comparison and assertions (lines 909–912).

This is essential to validate the PR's core promise: that parallel and sequential modes produce identical results.

I can generate the complete test using a realistic sample Metaschema document if you provide a reference or test fixture path.

---

933-945: Verify ParallelValidationConfig backward-compatibility constructor behavior.

The test at lines 933–945 validates that both the new constructor (with ParallelValidationConfig) and the old constructor (handler only) work. However, it does not verify:

1. That the old constructor correctly defaults to SEQUENTIAL config.
2. That sequential and parallel validators produce identical findings on simple documents.
3. That exception handling is consistent between the two constructors.

Add assertions to verify the sequential default and consider adding a comparative validation test.

---

1315-1331: Integration tests are stubs and lack concrete implementations.

The integration tests in Task 7 (lines 1315–1331) are mostly empty placeholders:

• Line 1315–1318: No actual CLI invocation or assertion.
• Line 1321–1324: Comments describe intent but no test code.
• Line 1327–1330: Performance test lacks baseline comparison.

Provide full implementations including:

1. Actual CLI command invocation (e.g., using ProcessBuilder or a test CLI utility).
2. Assertion of SARIF output correctness.
3. Performance baseline comparison (if this is a requirement).
4. Sample document creation or reference to existing test fixtures.

Without these, the integration test layer is not functional.

I can generate complete integration tests with CLI invocation and SARIF output validation if you provide paths to sample documents or test fixtures.

---

1353-1364: Verification checklist should include performance and correctness metrics.

The checklist (lines 1353–1364) covers compilation and option parsing but lacks:

1. Quantitative performance metrics: How much faster should parallel be on typical documents? (2×, 4×, etc.)
2. Correctness validation: Explicit statement that SARIF outputs from sequential and parallel modes are byte-for-byte identical or semantically equivalent.
3. Scalability limits: What is the maximum thread count tested? Does it scale linearly?
4. Stress testing: Large document validation (1000+ sibling nodes) to trigger parallel threshold.
5. Regression testing: Validation that existing non-parallel tests still pass.

Recommend adding these metrics to the checklist to ensure quality gates are clear.

PRDs/20251231-saxon-jdom2-removal/implementation-plan.md (3)

143-143: Provide concrete guidance for parameterized test approach.

The plan mentions parameterizing tests to validate both JDOM2 and new implementations (line 143: "or parameterize to test both"), but doesn't detail the expected test structure (e.g., parameterized fixture, abstract test base, test inheritance pattern). Provide an example or reference to clarify the expected test organization for reviewers.

---

264-272: Add performance regression testing to Phase 6 verification.

The Phase 6 final verification includes semantic equivalence checks (comparing generated schema output before/after) but doesn't address performance implications of migrating from JDOM2/Saxon to standard DOM/XPath APIs. Consider adding guidance for benchmarking or profiling schema generation performance (execution time, memory usage) to detect regressions.

---

290-301: Clarify public API visibility for new implementations.

The plan lists new classes (IndentingXMLStreamWriter, XmlSchemaLoader, DomDatatypeContent) under the .impl package, which conventionally indicates internal APIs. Explicitly state in the acceptance criteria for each new class whether it is:

• Package-private: Available only within dev.metaschema.schemagen.xml.impl (recommended for internal wrappers)
• Public: Part of the stable public API (requires API contract documentation)

core/src/main/java/dev/metaschema/core/metapath/cst/AbstractExpressionVisitor.java (1)

84-93: Minor typo in Javadoc.

Line 89: "whoose" should be "whose"

🔎 Proposed fix

   * Visit each child expression of the provided {@code expr}, aggregating the
   * results.
   *
   * @param expr
-  *          the expression whoose children should be visited
+  *          the expression whose children should be visited
   * @param context
   *          used to pass additional state
   * @return the aggegated result

.github/workflows/build.yml (1)

276-276: LGTM! TIMEOUT pattern correctly fixed.

The grep pattern now matches ^\[TIMEOUT\], consistent with the ERRORS pattern on line 275. This addresses the previous review feedback and correctly matches lychee's markdown output format.

The elif logic prioritizes errors over timeouts, which is reasonable since broken links are more critical. Note that if both errors and timeouts exist, the timeout count won't appear in the summary (though it remains in the report artifact).

Optional: Consider showing both metrics

If you want to display both errors and timeouts in the summary when both occur, you could replace the elif with separate if statements:

-          elif [ "$TIMEOUTS" -gt 0 ]; then
+          fi
+          if [ "$TIMEOUTS" -gt 0 ]; then
             echo ":warning: **$TIMEOUTS link(s) timed out** (external sites may be slow)" >> $GITHUB_STEP_SUMMARY
+            echo "" >> $GITHUB_STEP_SUMMARY
-          else
+          fi
+          if [ "$ERRORS" -eq 0 ] && [ "$TIMEOUTS" -eq 0 ]; then
             echo ":white_check_mark: **All links valid**" >> $GITHUB_STEP_SUMMARY
           fi

This would show both the error count and timeout count when both are present. The current approach is also valid if you prefer to highlight only the most critical issue.

Also applies to: 283-284

PRDs/20251231-choice-required-fix/implementation-plan.md (3)

51-63: Javadoc updates should clarify null-safety contract.

The plan to enhance setter Javadoc with nullability guidance is good. Ensure the updated getter Javadoc for isRequired() explicitly states: "Returns false for properties inside choice blocks, as their requirement is conditional on the choice branch being selected." This prevents future misinterpretations.

---

84-105: Acceptance criteria are well-defined but could be more specific on verification.

The criteria correctly require testing of null-safety annotations on generated code and choice property handling. Add explicit checkpoints: (1) verify regenerated classes have @Nullable on all choice property getters, and (2) run integration tests with the regenerated bootstrap classes to catch any annotation-driven behavior changes.

---

106-118: Files changed summary uses correct dev/metaschema paths.

All file paths reflect the PR #618 package rename to dev.metaschema.*. The summary correctly identifies modified, new, and regenerated files. Consider adding a note that the regenerated files (lines 116-118) will be automatically updated and should not be manually edited—improvements should flow through the code generator.

PRDs/20251217-parallel-constraint-validation/implementation-plan.md (1)

1015-1088: Parallel traversal strategy with threshold-based dispatch is sound.

The plan:

• Uses a PARALLEL_THRESHOLD constant (4) to decide when to parallelize (line 1041–1042).
• Avoids parallelization overhead for small child counts (sequential for < 4).
• Submits each child as independent task (lines 1054–1059).
• Collects futures and waits with future.get(), propagating exceptions (lines 1063–1078).
• Handles ExecutionException, InterruptedException, and cancels remaining futures on failure (lines 1067–1087).

The threshold of 4 is reasonable (avoids overhead for tiny sibling groups). The exception handling correctly distinguishes RuntimeException (re-throws) from checked exceptions (wraps in ConstraintValidationException). Good defensive coding on interruption (line 1076 restores interrupt flag).

One note: Consider whether context.subContext() is idempotent and lightweight (line 1056)—the plan mentions it copies executionStack and letVariableMap, which is O(n). For deeply nested documents with many siblings, this could add overhead. Monitor performance in practice; if problematic, consider a more efficient copy strategy or copy-on-write semantics in a future iteration.

PRDs/20251217-context-functions/PRD.md (1)

349-350: Clarify DefaultFunctionLibrary updates split between PR 2 and PR 3.

Line 349 lists DefaultFunctionLibrary.java as modified in PR 2 (for FnPosition/FnLast), but line 369 also lists the same file for PR 3 (for FnDefaultLanguage). This is likely intentional (both functions register in the same library), but the phasing should clarify this shared dependency to prevent integration surprises.

Consider either:

1. Consolidating both function registrations in PR 2 with a forward reference, or
2. Explicitly noting that PR 3 adds the FnDefaultLanguage registration to the library created in PR 2.

Also applies to: 369-369

PRDs/20251231-saxon-jdom2-removal/implementation-plan.md (3)

123-150: Clarify test parameterization strategy for JDOM2 → DOM migration.

The Phase 2 approach (write characterization tests against JDOM2, then verify new DOM implementation passes same tests) is sound, but the plan should clarify how tests will handle namespace-context differences between the two APIs. Mixed content documents can be polluted with indentation characters if the indentation logic is not carefully designed, which applies equally to XPath query results that may vary in whitespace handling.

For example, JDOM2 uses Namespace objects while javax.xml.xpath requires a NamespaceContext implementation (lines 396-411). Consider:

• Will tests use a single parameterized suite with adapter classes, or separate test classes?
• How will whitespace-sensitive assertions be handled?
• Are there XPath queries (e.g., text node selection) that behave differently between the APIs?

This clarification will prevent implementation surprises.

---

318-364: Minor pseudocode clarity: document the push/reset sequence for state tracking.

The stack-based state tracking design is sound, but the order of operations in the writeStartElement pseudocode (lines 334-340) should explicitly document when parent state is saved relative to delegation and depth increment. The current structure (save, delegate, increment, reset) is correct, but a simple comment clarifying the sequence would prevent implementation misinterpretation:

writeStartElement:
  if (!hasText) write newline + indent  // indent before tag
  hasTextStack.push(hasText)            // SAVE parent state first
  delegate.writeStartElement(...)       // delegate to underlying writer
  depth++                                // track new level
  hasText = false                       // reset for child context

This ensures the stack correctly preserves each ancestor's mixed-content state.

---

264-272: Expand Phase 6 Final Verification with more concrete checks.

Phase 6 acceptance criteria (lines 266-272) are minimal. Add explicit verification steps such as:

• Verify no remaining JDOM2/Saxon imports in compiled bytecode (class files)
• Compare generated XSD output byte-for-byte or semantically before/after to confirm indentation equivalence
• Run integration tests (if any) that depend on the schemagen module
• Verify Maven plugin invocations work end-to-end with new implementation

Additionally, the PR objectives mention "migration guidance" for downstream projects—consider adding a note in Phase 6 about updating README or migration docs (if applicable to this PR scope).

PRDs/20251217-cli-processor-refactor/implementation-plan.md (1)

1048-1051: Add @NonNull annotations to test utility method signature.

Line 1048 defines createContext() without @NonNull annotations, while the method always returns a non-null CallingContext and accepts non-null varargs. For consistency with other code in the project and to support static analysis, add annotations:

  @NonNull
  private CallingContext createContext(@NonNull String... args) {
    return new CallingContext(processor, Arrays.asList(args));
  }

As per coding guidelines, based on learnings, this aligns with the project's null-safety practices.

PRDs/20251215-path-formatters/plan.md (2)

294-294: Clarify task deferral status for JSON value key resolution helper.

Line 294 marks the JSON value key resolution helper as deferred ([ ] unchecked) with note "not needed for basic path formatting." However, the Phase 4 task list (lines 305–315) does not explicitly call out this deferral. If this task is intentionally skipped, confirm it won't be needed for Phase 4 implementation or clarify whether it should be added as a future Phase 5 task.

Consider adding a note in the Phase 4 task list (after line 315) clarifying which Phase 2 tasks are deferred and why, to prevent confusion during implementation.

Also applies to: 305-315

---

1-476: PRD is well-structured and properly aligned with package rename.

Overall assessment: This is a comprehensive and well-organized PRD that correctly applies the new dev.metaschema.* package naming convention throughout (per PR #618). The document clearly separates four implementation phases, provides detailed technical specifications with code examples, includes a robust testing strategy with ~50 test cases across phases, and identifies risks with mitigations. The phase-based task breakdown with checkmark completion tracking is clear.

Strengths:

• Clear problem statement and non-goals that prevent scope creep
• Detailed technical specifications for both XPathFormatter and JsonPointerFormatter with examples
• Comprehensive test matrix covering normal cases, edge cases, and special characters
• Risk/mitigation analysis shows forward-thinking about potential issues
• Proper use of dev.metaschema.* package namespace throughout

Minor areas for improvement (optional, per "Chill" review mode):

• Deferral of the JSON value key resolution helper (line 294) is noted but not explicitly cross-referenced in the Phase 4 task list
• No explicit mention of integration test approach for CLI in Phase 4 (test infrastructure section at lines 470–475 references Maven Invoker but Phase 4 tasks don't detail this)

core/src/main/java/dev/metaschema/core/metapath/function/CalledContext.java (1)

86-91: Consider consistent field access style.

The hashCode method uses getFunction().hashCode() while equals (line 102) accesses the function field directly. While functionally equivalent, consider using consistent field access for stylistic uniformity.

🔎 Optional refactor

   @Override
   public int hashCode() {
     final int prime = 31;
     int result = 1;
-    result = prime * result + getFunction().hashCode();
+    result = prime * result + function.hashCode();
     return prime * result + Objects.hash(contextItem, arguments);
   }

.github/workflows/build.yml (1)

237-241: LGTM! Consistent with the Code workflow.

This pre-build step mirrors the one in the Code workflow and correctly ensures the local plugin is available for the website build.

Optional: Extract to a reusable composite action

To reduce duplication, consider extracting this pre-build step into a reusable composite action that both workflows can reference. This would centralize the logic and make future maintenance easier. However, given the simplicity of the step, the current duplication is acceptable.